Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch to Centralized Package Management #798

Merged
merged 3 commits into from
Aug 18, 2024
Merged

Switch to Centralized Package Management #798

merged 3 commits into from
Aug 18, 2024

Conversation

decriptor
Copy link
Contributor

@decriptor decriptor commented Aug 17, 2024 •
edited
Loading

I'm not sure if this is a change you'd like in the project. For the most part this only moves the package versions to the Directory.Packages.props file. There was maybe one or two places where the packages versions differed slightly, and I picked the newer version.

I wasn't able to run the app locally with or without these changes because of a System.IndexOutOfRangeException being thrown when trying add generic.xaml to the MergedDictionaries:

public MainWindow()
{
    InitializeComponent();

    var uri = new Uri("StructuredLogViewer;component/themes/Generic.xaml", UriKind.Relative);
    var generic = new ResourceDictionary { Source = uri };
    Application.Current.Resources.MergedDictionaries.Add(generic);

This'll most likely cause problems with #782. Once that PR is in, I can fix this one.

@KirillOsenkov
Copy link
Owner

Thanks! Are you running on Mac or Windows? Note that StructuredLogViewer.csproj will only work on Windows with WPF, for Mac you want the Avalonia csproj.

@KirillOsenkov
Copy link
Owner

Apologies for creating merge conflicts, but the Avalonia PR is more valuable and a lot of hard work went into that, so I gave it priority.

Was there an issue that you thought Central Package Version Management will fix? Or is it just clean-up level?

Not sure what's going on with the IndexOutOfRange, that code is for loading the dark theme.

@decriptor
Copy link
Contributor Author

No worries. The Avalonia PR definitely needed to go in first.

This is mostly cleanup, but it does help clean up package versions and better keeps them in sync. There were a couple out of sync.

I'm planning on throwing a few more PRs up that are probably more "general" maintenance to start.

I'll try and update the PR this evening.

@decriptor
Copy link
Contributor Author

I'm testing this out on Windows and dark theme.

@KirillOsenkov
Copy link
Owner

Actually I took the liberty to resolve the conflicts already, I pushed to your branch, will make sure it works and merge. Thanks!

@KirillOsenkov KirillOsenkov merged commit 56b2f50 into KirillOsenkov:main Aug 18, 2024
1 check was pending
@decriptor
Copy link
Contributor Author

Actually I took the liberty to resolve the conflicts already, I pushed to your branch, will make sure it works and merge. Thanks!

Thanks!

@decriptor decriptor deleted the use-centralized-package-management branch August 18, 2024 01:16
@KirillOsenkov
Copy link
Owner

FYi @decriptor dotnet/wpf#9582

@decriptor
Copy link
Contributor Author

Thanks! Good to know

Numpsy
Numpsy reviewed Sep 3, 2024
View reviewed changes
src/StructuredLogger/StructuredLogger.csproj
<PackageReference Include="DotUtils.StreamUtils.Sources" Version="0.0.7" PrivateAssets="all" />
<PackageReference Include="Microsoft.Build.Framework" />
<PackageReference Include="Microsoft.Build.Utilities.Core" />
<PackageReference Include="Microsoft.SourceLink.GitHub" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that the NuGet package for MSBuild.StructuredLogger 2.2.317 is showing a dependency on Microsoft.SourceLink.GitHub 8.0.0, and I wonder if that was caused by the removal of PrivateAssets="all" here and whether that was intentional or not?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not intentional, I need to fix

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Published https://www.nuget.org/packages/MSBuild.StructuredLogger/2.2.337, thanks for spotting this

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Contributor Author

@decriptor decriptor Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies. Sorry for that.

@KirillOsenkov I was looking through the <TargetFramework(s)> and noticed that most of them are net8.0 with a couple others (net472, netstandard2.0, netcoreapp3.1). Is there any reason to not moving everything over to net8.0? (Or net9.0 in the near future?)

If we could move everything to net8.0 we could completely remove the SourceLink PackageReference: https://github.com/dotnet/sourcelink/releases/tag/8.0.0

If so, let me know and I'll create a PR for this work.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might not need to actually change the TFMs to use the inbox SourceLink - just ensuring it's all built with the .NET 8.0 SDK might be sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Numpsy I'm not sure either. I err'ed on the side of it might be more beneficial to just move everything to net8.0?

The one thing I did forget about though is that this repo does generate some nuget packages, so to better support everyone out there it might not be practical to drop net472 or netstandard2.0?

I guess the only way to know for sure would be to test it out :) I'll see if I can do that this evening or in the near future because I've been wondering about that anyways.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My request is to minimize contributions that don't solve real problems - maintaining this repo (among dozens of other repos I maintain) is already taking a huge toll on me, and PRs that look like help actually just cause more work for me in the future.

You can read here for a recent example:
#801 (comment)

So ideally if you feel that there's a real problem with the app, file an issue, and for issues that I need help with, I can mark them as up-for-grabs, and then feel free to send PRs for those.

My time and attention is incredibly scarce and valuable, so ideally I'd minimize interruptions and disruptions and pings as respect for my time and what I'm already doing for the community.

Thanks for your understanding!

@Numpsy Numpsy mentioned this pull request Sep 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Numpsy Numpsy Numpsy left review comments

@KirillOsenkov KirillOsenkov KirillOsenkov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@decriptor @KirillOsenkov @Numpsy